Add method to track entity state changes from target selectors#148086
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
| ) | ||
| return lambda: None | ||
|
|
||
| tracker = TargetSelectorStateChangeTracker(hass, selector_data, job_type, action) |
There was a problem hiding this comment.
Initially I had a functional implementation, but I wasn't a big fan of it so I moved it into a class. I think it is easier to read now.
The functional version is here: 5d553e5 (#148086)
There was a problem hiding this comment.
I'm not sure this function adds value, why don't users just create a TargetSelectorStateChangeTracker themselves?
There was a problem hiding this comment.
They would also have to create a TargetSelectorData too. I think the function makes it a bit simpler to use (and in a way that is similar to the other event tracking methods).
But I am ok with removing. Let me know if you think it is not worth it.
| ) | ||
| return lambda: None | ||
|
|
||
| tracker = TargetSelectorStateChangeTracker(hass, selector_data, job_type, action) |
There was a problem hiding this comment.
I'm not sure this function adds value, why don't users just create a TargetSelectorStateChangeTracker themselves?
| # List of entities that should assert a state change when toggled. Contrary to | ||
| # entities_to_set_state, entities should be added and removed. | ||
| entities_to_assert_change = [] |
There was a problem hiding this comment.
Remove this list, and instead pass it to assert_entity_calls_and_reset to make the test easier to follow
There was a problem hiding this comment.
I had it implemented like that initially and I don't think it was making the test easier to follow, that is why I changed to this.
With entities_to_assert_change you can clearly see what gets added/removed on each "step" of the test. Also, it ensures that previous entities keep getting tested, unless explicitly removed.
With a full list being passed every it is easier to miss passing an entity, and also makes it a bit harder to see what changed from step to step as you have to read the whole list.
There was a problem hiding this comment.
With entities_to_assert_change you can clearly see what gets added/removed on each "step" of the test
This is precisely what I don't think it's good, because to understand what happens at a certain line in the test, all previous lines of the test must be read too
Also, it ensures that previous entities keep getting tested, unless explicitly removed
Why is that? assert_entity_calls_and_reset checks the length of the entities_to_assert_change list, so if an item is forgotten, that check will fail, no?
There was a problem hiding this comment.
This is precisely what I don't think it's good, because to understand what happens at a certain line in the test, all previous lines of the test must be read too
If the full list is always passed to the method, you still have to check the previous lines to make sure you copied all the relevant entities again. And you have to check all steps, to ensure that you keep only the ones that are still triggered. The way it is currently you can look at each step and see "entity X is removed -> check it" or "entity X is now added -> checked from now one", without having to check every step.
Why is that?
assert_entity_calls_and_resetchecks the length of theentities_to_assert_changelist, so if an item is forgotten, that check will fail, no?
That is true when there is are no bugs, which is what the test should prevent.
For example, lets say currently entity_a is tracked. Then the code gets a new feature where some new target gets tracked, which introduces a bug and entity_a stops being tracked as soon as that target is tracked. On the test, you track the new target as another step.
- with the current implementation you would have to explicitly add
entities_to_assert_change.remove(entity_a)on that step. - if instead we pass the full list it would make it easy to miss entity_a in the full list, unless you read the full test again.
| # List of entities to toggle state during the test. This list should be insert-only | ||
| # so that all entities are changed every time. | ||
| entities_to_set_state = [] |
There was a problem hiding this comment.
| # List of entities to toggle state during the test. This list should be insert-only | |
| # so that all entities are changed every time. | |
| entities_to_set_state = [] |
|
|
||
| targeted_entity = "light.test_light" | ||
|
|
||
| entities_to_set_state.extend([targeted_entity, device_entity, untargeted_entity]) |
There was a problem hiding this comment.
| entities_to_set_state.extend([targeted_entity, device_entity, untargeted_entity]) | |
| # List of entities to toggle state during the test. This list should be insert-only | |
| # so that all entities are changed every time. | |
| entities_to_set_state = [targeted_entity, device_entity, untargeted_entity] |
There was a problem hiding this comment.
since this is used in toggle_states(), I think it is easier to follow the code if the variable is declared before the method that uses it.
The performance impact of using extend() here isn't really an issue.
There was a problem hiding this comment.
It's not about performance, it's about the mental load of tracking the mutations of entities_to_set_state while reading the test
There was a problem hiding this comment.
it's about the mental load of tracking the mutations of
entities_to_set_statewhile reading the test
I am not sure I get how that mental load gets reduced by moving the declaration of the variable down from the top to here. What is the difference?
Co-authored-by: Erik Montnemery <erik@montnemery.com>
2 participants
Proposed change
This PR adds a method to allow triggers to subscribe to events from target selector targets (like floors, labels, etc.).
Requires (and includes commits from): #148087Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: